Walkthrough

Upgrade Shakapacker to 9.1.0 and add runtime bundler selection (webpack ↔ rspack): add rspack deps and patch-package, refactor webpack configs to select bundler at runtime and generate per-call configs, switch CLI to Shakapacker::Runner, change SWC React runtime to classic, add tests and docs for bundler switching.

Changes

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Dev as Developer
  participant Bin as bin/shakapacker
  participant Runner as Shakapacker::Runner
  participant Bundler as Node Bundler (webpack / @rspack/core)

  Dev->>Bin: run shakapacker [args]
  Bin->>Runner: Runner.run(ARGV)
  Runner->>Bundler: determine & invoke bundler-specific build
  Bundler-->>Runner: exit status
  Runner-->>Dev: propagate status

sequenceDiagram
  autonumber
  participant Node as Node.js
  participant Config as shakapacker config
  participant WebpackCfg as webpack config module
  participant Bundler as webpack / @rspack/core

  Node->>WebpackCfg: require(...)
  WebpackCfg->>Config: read config.assets_bundler
  Config-->>WebpackCfg: "rspack" or "webpack"
  alt assets_bundler == "rspack"
    WebpackCfg->>Bundler: require("@rspack/core")
  else
    WebpackCfg->>Bundler: require("webpack")
  end
  WebpackCfg->>Bundler: use bundler.*Plugin APIs (ProvidePlugin, DefinePlugin, etc.)
  WebpackCfg-->>Node: export final config

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

• Migrate from Webpack to Rspack #668 — Matches migration to Rspack and assets_bundler switching implemented here.
• Documentation: Add guide for migrating to Rspack with Shakapacker 9 react_on_rails#1863 — Overlaps on bundler auto-detection, CSS/SSR adjustments, .bs.js ReScript support and docs.

Possibly related PRs

• Migrate to Shakapacker 9.0.0-beta.8 #665 — Overlaps on SCSS/sass-loader detection and commonWebpackConfig refactor.
• Migrate to Shakapacker 9.0.0-beta.7 with Babel transpiler #662 — Related css-loader / CSS Modules option adjustments.
• Switch from Babel to SWC for faster JavaScript transpilation #677 — Related SWC React transform and bundler configuration changes.

Suggested reviewers

• ahangarha
• AbanoubGhadban

Poem

Hop hop, I tweak the pack tonight,
Swapped the runner, set bundlers right.
Rspack dances, webpack nods along,
Classic JSX hums a carrot song.
.bs.js shines — build bells ring, hooray! 🥕✨

Pre-merge checks and finishing touches

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

🚀 Quick Review App Commands

Welcome! Here are the commands you can use in this PR:

/deploy-review-app

Deploy your PR branch for testing

/delete-review-app

Remove the review app when done

/help

Show detailed instructions, environment setup, and configuration options.

Pull Request Review: Shakapacker 9.1.0 & Rspack Migration

Summary

This PR upgrades Shakapacker from 9.0.0-beta.8 to 9.1.0 stable and migrates the bundler from webpack to Rspack. Overall, this is a well-executed migration with proper configuration structure. Below are my findings:

✅ Strengths

1. Excellent Configuration Structure: The Rspack configuration mirrors the webpack structure perfectly, making it easy to understand and maintain
2. Complete Migration: All necessary configuration files (client, server, development, production, test) have been properly created
3. Backward Compatibility: Webpack configs remain intact for reference or rollback
4. Performance Benefits: Rspack promises 2-10x faster cold builds and 5-20x faster incremental builds
5. Proper SSR Handling: Server-side rendering configuration is correctly implemented with appropriate optimizations
6. Good Documentation: The PR description clearly explains changes and testing performed

⚠️ Issues & Recommendations

1. Ruby Version Downgrade (❗️ High Priority)

Location: Gemfile and Gemfile.lock

- ruby "3.4.6"
+ ruby "3.4.3"

Issue: The Ruby version was downgraded from 3.4.6 to 3.4.3 without explanation.

Questions:

• Was this intentional or accidental?
• Is there a compatibility issue with Shakapacker 9.1.0 and Ruby 3.4.6?
• If intentional, this should be documented in the PR description

Recommendation: Either revert to 3.4.6 if the downgrade was unintentional, or document the reason for the change.

2. Mutable Global Configuration (⚠️ Medium Priority)

Location: config/rspack/commonRspackConfig.js:4-69

Issue: The code directly mutates baseClientRspackConfig before wrapping it in a function:

const baseClientRspackConfig = generateWebpackConfig(); // Global mutable object
// ... direct mutations to baseClientRspackConfig ...
const commonRspackConfig = () => merge({}, baseClientRspackConfig, commonOptions, ignoreWarningsConfig);

Problem: Even though the final function uses merge({}, ...), the baseClientRspackConfig object is mutated during module load time (lines 23-66). This means:

• All mutations happen once when the module is first loaded
• Multiple calls to commonRspackConfig() share the same mutated base object
• This could cause issues if the configuration is generated multiple times

Recommendation: Refactor to avoid mutations:

const commonRspackConfig = () => {
  const baseClientRspackConfig = generateWebpackConfig();
  
  // Apply mutations here inside the function
  const scssConfigIndex = baseClientRspackConfig.module.rules.findIndex(...);
  // ... rest of the mutation logic ...
  
  return merge({}, baseClientRspackConfig, commonOptions, ignoreWarningsConfig);
};

This pattern is mentioned in the comments but not fully implemented. The same issue exists in the webpack config, so this could be addressed as a follow-up refactor for both.

3. Inconsistent Error Handling (⚠️ Low Priority)

Location: config/rspack/serverRspackConfig.js:19-23

if (!serverEntry['server-bundle']) {
  throw new Error(
    "Create a pack with the file name 'server-bundle.js' containing all the server rendering files",
  );
}

Issue: The error message is user-facing but doesn't explain what caused the problem or where to look.

Recommendation: Improve error message:

if (!serverEntry['server-bundle']) {
  throw new Error(
    "Missing 'server-bundle' entry in webpack/rspack configuration. " +
    "Please create a pack file named 'server-bundle.js' in your source entry path " +
    "containing all server rendering files."
  );
}

4. Code Duplication Between Webpack and Rspack Configs (📝 Low Priority)

Location: config/webpack/ vs config/rspack/

Observation: The configuration logic is nearly identical between webpack and rspack (particularly commonWebpackConfig.js vs commonRspackConfig.js).

Recommendation: Consider:

• Documenting which configuration is authoritative going forward
• Creating a migration plan to remove webpack configs if they're no longer needed
• Or keeping them both documented as valid alternatives

This isn't urgent but should be clarified for future maintainers.

5. Missing webpack-dev-server Rspack Equivalent Check (ℹ️ Info)

Location: package.json:144

Observation: The project still has webpack-dev-server: ^4.11.1 but added @rspack/dev-server via the @rspack/cli dependency.

Question: Does Rspack use its own dev server or the webpack dev server? The dependencies suggest both might be present.

Recommendation: Verify that the correct dev server is being used and consider removing unused dependencies in a follow-up.

🔒 Security Considerations

✅ No security concerns identified. The migration:

• Uses official, maintained packages (@rspack/core, @rspack/cli)
• Doesn't introduce any suspicious dependencies
• Maintains existing security configurations
• Doesn't expose any sensitive configuration

🧪 Test Coverage

Strengths:

• ✅ Development, production, and test builds tested
• ✅ RuboCop linting passed

Suggestions:

• Consider adding a test case that verifies both client and server bundles build successfully
• Document any changes in build performance (actual measurements would be valuable)
• Verify HMR (Hot Module Replacement) works correctly in development

🎯 Performance Considerations

✅ Excellent: The migration to Rspack should provide significant performance improvements. The configuration properly:

• Disables source maps optimization for SSR (using 'eval' for speed)
• Limits chunks appropriately for server bundles
• Maintains proper CSS extraction for client bundles

📋 Additional Notes

1. bin/shakapacker Changes: The binstub was properly updated to use the new Shakapacker::Runner instead of Shakapacker::WebpackRunner ✅

2. shakapacker.yml Configuration: The assets_bundler: rspack setting is correctly placed ✅

3. Dependency Versions: All Rspack packages use consistent versions (^1.5.8) ✅

🎬 Verdict

Overall Assessment: ✅ Approve with Minor Changes Recommended

This is a well-executed migration that follows best practices. The two main items to address before merging:

1. Must Fix: Clarify/fix the Ruby version downgrade
2. Should Fix: Consider refactoring the mutable global configuration pattern (or document it as technical debt to address later)

The other issues are minor and could be addressed in follow-up PRs.

Great work on this migration! 🚀

Pull Request Review: Update to Shakapacker 9.1.0 and migrate to Rspack

Overall Assessment

This is a well-executed migration from webpack to Rspack with Shakapacker 9.1.0. The PR is comprehensive and maintains backward compatibility. The code quality is generally good, but there are a few areas that could be improved.

✅ Strengths

1. Thorough Documentation: The PR description is excellent with clear summary, testing details, and performance expectations
2. Complete Configuration Migration: All necessary Rspack configuration files have been created mirroring the webpack setup
3. Backward Compatibility: The webpack configurations are retained, providing a safe rollback path
4. Testing Coverage: Build testing across all environments (development, test, production) has been performed

🔍 Code Quality Issues

Critical Issues

1. Ruby Version Downgrade (Gemfile:6)

-ruby "3.4.6"
+ruby "3.4.3"

❌ This is a concerning change. Why was the Ruby version downgraded from 3.4.6 to 3.4.3? This appears unrelated to the Rspack migration and could:

• Introduce security vulnerabilities if 3.4.6 contains important fixes
• Cause issues in production if different environments use different Ruby versions
• Break compatibility with dependencies that require 3.4.6+

Recommendation: Revert this change or provide explicit justification in the PR description.

High Priority Issues

2. Missing Error Handling in Configuration Files

config/rspack/commonRspackConfig.js:23-66

• The SCSS configuration logic has complex nested conditionals but no error recovery
• If cssLoader.options.modules is unexpectedly undefined, line 62 will throw
• The sass-loader modification assumes the loader exists but doesn't validate

Recommendation: Add defensive checks:

if (cssLoader?.options?.modules?.namedExport) {
  if (!cssLoader.options.modules.exportLocalsConvention) {
    cssLoader.options.modules.exportLocalsConvention = 'camelCaseOnly';
  }
}

3. Potentially Unsafe CSS Loader Configuration

config/rspack/serverRspackConfig.js:95-96

if (cssLoader && cssLoader.options) {
  cssLoader.options.modules = { exportOnlyLocals: true };
}

❌ This overwrites the entire modules configuration instead of merging. This could lose existing module configuration like namedExport, localIdentName, etc.

Recommendation:

if (cssLoader && cssLoader.options) {
  cssLoader.options.modules = {
    ...cssLoader.options.modules,
    exportOnlyLocals: true
  };
}

4. Incomplete Type Safety for Loader Checks

config/rspack/serverRspackConfig.js:100-102

• Checks for url-loader and file-loader but doesn't verify rule.use.options exists before setting emitFile
• Could throw if options is undefined

Recommendation:

} else if (rule.use && (rule.use.loader === 'url-loader' || rule.use.loader === 'file-loader')) {
  rule.use.options = rule.use.options || {};
  rule.use.options.emitFile = false;
}

Medium Priority Issues

5. New i18n Files Appear Unused or Untested

client/app/libs/i18n/default.js and client/app/libs/i18n/translations.js

• These files appear to already exist based on the grep results showing existing imports
• The PR description doesn't mention i18n changes
• If these are new files, there's no evidence they're being tested or used

Recommendation:

• If these files already exist, they shouldn't show as "new" in the diff
• If they're replacing existing files, add migration notes
• Add tests for the i18n functionality

6. Hardcoded Path in sass-resources-loader

config/rspack/commonRspackConfig.js:15

resources: './client/app/assets/styles/app-variables.scss',

• Uses a relative path that could break depending on execution context
• Should use path.resolve(__dirname, ...) for consistency with other paths in the config

Recommendation:

const path = require('path');
// ...
resources: path.resolve(__dirname, '../../client/app/assets/styles/app-variables.scss'),

7. Missing Package Version Constraints

package.json:111-113

"@rspack/cli": "^1.5.8",
"@rspack/core": "^1.5.8",

• Using caret ranges for critical bundler dependencies
• If Rspack 1.6.0 introduces breaking changes, builds could fail unexpectedly

Recommendation: Consider using exact versions or tilde ranges for more predictable builds, especially for a bundler migration.

🔒 Security Considerations

No Critical Security Issues Found

• Dependencies are from official npm registry
• No obvious code injection vulnerabilities
• Configuration files don't expose secrets

Minor Concern: The ignoreWarnings configuration in commonRspackConfig.js:20 suppresses a React DOM client warning. Ensure this warning is genuinely expected and not masking a real issue.

⚡ Performance Considerations

Positive:

• Rspack's performance benefits are well-documented (2-10x faster builds)
• Configuration appears optimized (modern sass API, proper CSS modules setup)
• Server bundle optimization disables minimization appropriately for SSR

Suggestions:

1. Consider adding build time benchmarks to the PR description
2. Monitor production bundle sizes to ensure no unexpected increases

🧪 Test Coverage

Missing Tests:

• No automated tests for the new Rspack configuration files
• No integration tests verifying SSR still works with Rspack
• No tests for the new i18n files (if they're actually new)

Recommendation: Add smoke tests that verify:

• Client bundle builds successfully
• Server bundle builds successfully
• SSR rendering still functions
• HMR works in development

📝 Documentation

Good:

• Comprehensive PR description
• Inline comments explaining configuration choices
• References to Rspack documentation

Missing:

• No update to project README about the bundler change
• No migration guide for other developers
• No documentation about when/why to use webpack vs rspack

🎯 Best Practices

Well Done:

• Consistent code style
• Logical file organization
• Proper use of environment-specific configs
• Good separation of concerns (client/server/common configs)

Could Improve:

• Add JSDoc comments to exported functions
• Consider extracting magic strings to constants
• Add validation for required configuration options

📊 Summary

Approve with Changes Requested

This PR represents solid engineering work on a complex migration. However, the Ruby version downgrade is a blocker that needs explanation, and the CSS loader configuration issues could cause runtime problems.

Required Changes:

1. ✅ Explain or revert Ruby version downgrade (3.4.6 → 3.4.3)
2. ✅ Fix CSS modules object spreading in serverRspackConfig.js
3. ✅ Add defensive checks in commonRspackConfig.js

Recommended Changes:

4. Use absolute paths for file resources
5. Add tests for critical build configurations
6. Document the bundler change in README
7. Verify i18n files are intentional additions

Nice to Have:

8. Add JSDoc comments
9. Include build time benchmarks
10. Consider stricter dependency versioning

Great work on this migration! Once the critical issues are addressed, this will be a valuable improvement to the project.

Review generated with Claude Code

Pull Request Review: Shakapacker 9.1.0 & Rspack Migration

Summary

This PR successfully upgrades Shakapacker from beta to stable (9.1.0) and migrates from webpack to Rspack. The migration is well-structured and comprehensive. Overall, this is a solid PR with good attention to detail. Below are my findings across different review categories.

✅ Strengths

1. Excellent Configuration Structure

• Well-organized Rspack configuration with proper separation of concerns (client, server, common, environment-specific)
• Thoughtful comments explaining complex decisions (e.g., mutable globals, SSR configurations)
• Proper handling of CSS modules with exportOnlyLocals: true for SSR

2. Backward Compatibility

• Non-breaking change that maintains full compatibility
• Old webpack configs preserved for reference
• Proper migration path documented

3. Modern Best Practices

• Modern sass-loader API configuration (api: 'modern')
• Proper devtool settings (eval for SSR for faster debugging)
• Correct handling of CSS in SSR (no style extraction on server bundle)

⚠️ Issues & Recommendations

High Priority

1. ESLint Configuration Needs Update

Location: .eslintrc:46-47

The ESLint webpack resolver is still pointing to the old webpack config:

webpack:
  config: 'config/webpack/development.js'

Recommendation: Update to point to Rspack config or remove if not needed:

webpack:
  config: 'config/rspack/development.js'

2. Unused i18n Files Without Integration

Locations:

• client/app/libs/i18n/default.js
• client/app/libs/i18n/translations.js

These new files define translations but:

• The translations export is never imported or used anywhere in the codebase
• defaultMessages is exported but never imported
• Only defaultLocale is actually used (in selectLanguage.jsx)
• The existing i18n system uses a different pattern (doesn't reference these files)

Recommendation:

• Option A: Remove these files if they were added by mistake
• Option B: If intentional, integrate them into the existing i18n system and document the migration plan
• Option C: Add a TODO comment explaining future integration plans

3. Potential Code Duplication in serverRspackConfig.js

Location: config/rspack/serverRspackConfig.js:27-36 and 67-83

The code filters out mini-css-extract-plugin from loaders in two places:

• Lines 27-36: First pass through loaders
• Lines 67-83: Second pass through rules

Recommendation: Consider consolidating into a single pass or add a comment explaining why two passes are necessary.

Medium Priority

4. Regex Matching Logic Could Be More Robust

Location: config/rspack/commonRspackConfig.js:23-24

const scssConfigIndex = baseClientRspackConfig.module.rules.findIndex((config) =>
  '.scss'.match(config.test) && config.use,
);

Issue: This assumes config.test is always a RegExp. If it's a string or function, this could fail silently.

Recommendation:

const scssConfigIndex = baseClientRspackConfig.module.rules.findIndex((config) =>
  config.test && config.test.test('.scss') && config.use,
);

5. Missing Error Handling

Location: config/rspack/commonRspackConfig.js:39-52

If sassLoaderIndex is found but the loader has unexpected structure, the code could fail.

Recommendation: Add defensive checks or wrap in try-catch with meaningful error messages.

6. Inconsistent Optional Chaining

Location: config/rspack/serverRspackConfig.js:95

The code uses optional chaining inconsistently. Line 95 uses cssLoader.options without optional chaining after checking cssLoader exists.

Recommendation: Use consistent patterns:

if (cssLoader?.options) {
  cssLoader.options.modules = { exportOnlyLocals: true };
}

Low Priority

7. Version Mismatch in Gemfile.lock

Location: Gemfile.lock

Ruby version changed from 3.4.6p54 to 3.4.6p32 - this appears to be a patch version downgrade.

Question: Is this intentional? Typically, patch versions should move forward, not backward.

8. Missing Type Safety

The Rspack configuration files are JavaScript without JSDoc comments. Consider adding JSDoc for better IDE support:

/**
 * @returns {import('@rspack/core').Configuration}
 */
const configureServer = () => {
  // ...
};

9. Console.warn vs Console.log

Location: config/rspack/commonRspackConfig.js:28

Uses console.warn for a non-warning message. Consider console.error or throw an error since missing SCSS config is likely a serious issue.

🔒 Security Considerations

✅ No security concerns identified. The changes are configuration-only and don't introduce new security vulnerabilities.

🚀 Performance Considerations

✅ Excellent performance improvements expected:

• Rspack provides 2-10x faster cold builds
• 5-20x faster incremental builds
• Proper code splitting maintained
• SSR optimization with eval devtool is a good choice

🧪 Test Coverage

⚠️ Concerns:

1. No visible test additions/updates for the new configuration
2. The i18n files added have no tests
3. Consider adding:
   • Integration tests verifying Rspack builds succeed
   • Tests confirming SSR still works correctly
   • Tests for the new i18n files (if they're meant to be used)

Note: The PR description mentions builds tested successfully, which is good. Consider adding automated tests to prevent regressions.

📋 Code Quality

Overall: Good (8/10)

Positives:

• Clean, readable code
• Good use of comments
• Follows existing patterns
• Proper error messages

Areas for improvement:

• More defensive programming
• Consistency in optional chaining
• Type annotations (JSDoc)

🎯 Action Items

Before merging:

1. ✅ Update or remove ESLint webpack resolver config
2. ✅ Address the unused i18n files (remove or integrate)
3. ✅ Clarify Ruby version change in Gemfile.lock

Nice to have:
4. Add JSDoc type annotations
5. Consolidate duplicate filtering logic
6. Add integration tests for Rspack builds
7. More robust regex matching for loader detection

💡 Final Recommendation

Conditional Approval - This is excellent work on a complex migration! The core changes are solid and well-thought-out. Please address the high-priority items (ESLint config and unused i18n files) before merging. The medium and low priority items can be addressed in follow-up PRs if preferred.

Great job on this migration! 🎉

🤖 Generated with Claude Code

Pull Request Review: Shakapacker 9.1.0 & Rspack Migration

I've completed a comprehensive review of PR #680. Overall, this is a well-executed migration from webpack to Rspack with Shakapacker 9.1.0. Here's my detailed feedback:

✅ Strengths

1. Excellent Documentation

   • The PR description is thorough and well-structured
   • Clear explanation of changes, performance benefits, and testing
   • Good use of commit messages to track incremental fixes

2. Comprehensive Configuration Migration

   • Complete Rspack configuration mirroring the webpack setup
   • Proper separation of client/server bundles
   • Environment-specific configurations (dev/prod/test)

3. SSR Compatibility

   • Thoughtful handling of server-side rendering requirements
   • Correct CSS modules configuration with exportOnlyLocals: true for SSR (config/rspack/serverRspackConfig.js:95-96)
   • Appropriate changes to React runtime (classic instead of automatic) for better SSR compatibility (config/swc.config.js:14)

4. Performance Focus

   • The switch to Rspack aligns with modern build tool trends
   • Expected 2-10x faster cold builds and 5-20x faster incremental builds

🔍 Issues & Concerns

Critical: Potential Bug in CSS Modules Configuration

Location: config/rspack/serverRspackConfig.js:95-96

if (cssLoader && cssLoader.options) {
  cssLoader.options.modules = { exportOnlyLocals: true };
}

Issue: This code overwrites the entire modules configuration object, potentially losing existing settings like namedExport, exportLocalsConvention, etc. that were configured in commonRspackConfig.js:61-62.

Impact: This could break CSS modules that rely on named exports or specific naming conventions.

Recommendation:

if (cssLoader && cssLoader.options && cssLoader.options.modules) {
  cssLoader.options.modules = {
    ...cssLoader.options.modules,
    exportOnlyLocals: true,
  };
}

Medium: Inconsistent CSS Loader Handling

Location: config/rspack/commonRspackConfig.js:61-62

The code checks for namedExport before setting exportLocalsConvention, but doesn't validate if modules itself is enabled:

if (cssLoader?.options?.modules?.namedExport) {
  cssLoader.options.modules.exportLocalsConvention = 'camelCaseOnly';
}

Recommendation: Add a check to ensure modules is enabled:

if (cssLoader?.options?.modules) {
  if (cssLoader.options.modules.namedExport) {
    cssLoader.options.modules.exportLocalsConvention = 'camelCaseOnly';
  }
}

Low: Code Duplication in serverRspackConfig.js

Location: Lines 30-36 and 72-82

The code filters out mini-css-extract-plugin twice in the same file. The second block (lines 72-82) appears more comprehensive as it also removes style-loader.

Recommendation: Consider consolidating this logic or adding a comment explaining why it's done in two places.

📝 Code Quality Suggestions

1. Error Handling Enhancement

   Location: config/rspack/serverRspackConfig.js:19-23

   The error message is good, but could benefit from more context:

   if (!serverEntry['server-bundle']) {
     throw new Error(
       "Missing 'server-bundle' entry point. Create a pack with the file name 'server-bundle.js' containing all the server rendering files. Check your entry points configuration."
     );
   }

2. Magic String in rspackConfig.js

   Location: Line 13 has a typo: the the

   // For HMR, need to separate the the client and server rspack configurations

   Should be:

   // For HMR, need to separate the client and server rspack configurations

3. Sass Loader Options Safety

   Location: config/rspack/commonRspackConfig.js:39-51

   Good defensive programming, but consider extracting the sass-loader configuration logic into a separate function for better testability.

🔒 Security Assessment

✅ No security concerns identified

• Dependencies are from trusted sources (@rspack/*, rspack-manifest-plugin)
• No credentials, API keys, or sensitive data in configuration files
• Proper use of environment-specific configurations

🧪 Test Coverage

Observations:

• PR description mentions successful testing of dev/test/prod builds ✅
• RuboCop linting passed ✅
• Missing: No automated tests for the new Rspack configurations themselves
• Missing: No tests verifying CSS modules work correctly in SSR context

Recommendations:

1. Add integration tests to verify bundle generation
2. Add tests to ensure CSS modules export correctly for both client and server bundles
3. Consider adding a test that validates the i18n files are properly bundled

⚡ Performance Considerations

Positive:

• Rspack is a significant performance upgrade
• Configuration properly disables optimization for server bundle (serverRspackConfig.js:39-41)
• eval devtool for server bundle is the right choice for fast SSR debugging (serverRspackConfig.js:108)

Consider:

• Monitor bundle sizes post-migration to ensure no regression
• Verify that HMR (Hot Module Replacement) works as expected in development
• The ignoreWarnings configuration (commonRspackConfig.js:19-21) suppresses React 18+ warnings - ensure this doesn't hide real issues

🎯 Additional Observations

1. i18n Files Addition

   • The addition of client/app/libs/i18n/default.js and translations.js appears intentional (separate commit)
   • These files are properly imported and used throughout the codebase ✅
   • Good addition for internationalization support

2. Ruby Version Change

   • Gemfile.lock shows Ruby version changed from 3.4.6p54 to 3.4.6p32
   • This might be environment-specific; ensure CI/CD environments are aligned

3. Webpack Configuration Retention

   • The PR mentions webpack configs can be kept for reference or removed later
   • Recommendation: Add a follow-up issue/PR to remove old webpack configs to avoid confusion

📋 Summary

Approval Status: ✅ Approve with minor fixes recommended

This is a solid migration with good attention to detail. The main concern is the CSS modules configuration bug in serverRspackConfig.js that could break existing functionality. I recommend:

1. Before merge: Fix the CSS modules configuration overwrite issue
2. Before merge: Fix the typo ("the the")
3. After merge: Add automated tests for bundle generation
4. After merge: Clean up old webpack configurations

The performance benefits of Rspack and the thorough configuration work make this a valuable upgrade. Great work on maintaining backward compatibility!

Review completed by: Claude Code
Reviewed on: $(date +%Y-%m-%d)

Code Review: Update to Shakapacker 9.1.0 and Migrate to Rspack

Summary

This PR successfully migrates from webpack to Rspack as the JavaScript bundler and upgrades Shakapacker from beta to stable. The migration is well-executed with comprehensive configuration files that mirror the webpack setup while taking advantage of Rspack-specific optimizations.

✅ Strengths

1. Comprehensive Configuration Migration

• All webpack configurations have been properly translated to Rspack equivalents
• Separate client/server bundle configurations maintain SSR functionality
• Environment-specific configs (development, production, test) are properly structured

2. Well-Documented Changes

• Clear commit messages explaining each change
• Inline comments explaining configuration decisions
• Good explanation of the performance benefits (2-10x faster builds)

3. SSR Compatibility Fixes

• Multiple commits addressing SSR issues show thorough testing
• CSS modules configuration properly preserves namedExport settings (commit 3fe61f0)
• React runtime switched to 'classic' for better SSR compatibility (commit 5d85f15)

4. Proper Dependency Management

• All required Rspack dependencies added
• Version constraints are appropriate
• Ruby version mismatch fixed for CI compatibility

🔍 Issues & Recommendations

High Priority

1. Missing File Path Validation (config/rspack/commonRspackConfig.js:15)

The SCSS resources path is hardcoded without validation:

resources: './client/app/assets/styles/app-variables.scss',

Recommendation: Add path validation or use path.resolve() with existence check to fail fast if the file is missing.

const path = require('path');
const fs = require('fs');
const appVariablesPath = path.resolve(__dirname, '../../client/app/assets/styles/app-variables.scss');
if (!fs.existsSync(appVariablesPath)) {
  throw new Error(`SCSS variables file not found: ${appVariablesPath}`);
}

2. Mutable Global Object Mutation (config/rspack/commonRspackConfig.js:65, 69)

The code mutates baseClientRspackConfig directly:

baseClientRspackConfig.module.rules[scssConfigIndex].use.push(sassLoaderConfig);
// ...
const commonRspackConfig = () => merge({}, baseClientRspackConfig, commonOptions, ignoreWarningsConfig);

Issue: The comment on line 69 says "Copy the object using merge b/c the baseClientRspackConfig and commonOptions are mutable globals", but the config is already mutated on line 65 before the merge.

Recommendation: Either:

• Move the mutation inside the function returned by commonRspackConfig, or
• Deep clone before mutation

3. Potential Null Reference (config/rspack/serverRspackConfig.js:104-106)

} else if (rule.use && (rule.use.loader === 'url-loader' || rule.use.loader === 'file-loader')) {
  rule.use.options.emitFile = false;
}

Issue: No check for rule.use.options existence before assignment.

Fix:

} else if (rule.use && (rule.use.loader === 'url-loader' || rule.use.loader === 'file-loader')) {
  rule.use.options = rule.use.options || {};
  rule.use.options.emitFile = false;
}

Medium Priority

4. Inconsistent Loader Finding Logic (config/rspack/commonRspackConfig.js:32-37 vs 56-59)

The code uses two different patterns for finding loaders in the same file:

• Lines 32-37: More defensive with type checking
• Lines 56-59: More concise with optional chaining

Recommendation: Standardize on one approach for consistency and maintainability.

5. Duplicate Code in serverRspackConfig.js

Lines 27-36 and lines 67-83 both filter out mini-css-extract-plugin and style-loader.

Recommendation: Extract to a helper function:

const removeStyleLoaders = (rules) => {
  rules.forEach((loader) => {
    if (loader.use && loader.use.filter) {
      loader.use = loader.use.filter(
        (item) => !(typeof item === 'string' && item.match(/mini-css-extract-plugin/)) && item !== 'style-loader'
      );
    }
  });
};

6. Hard-coded Plugin Names (config/rspack/serverRspackConfig.js:60-65)

plugin.constructor.name !== 'WebpackAssetsManifest' &&
plugin.constructor.name !== 'MiniCssExtractPlugin' &&
plugin.constructor.name !== 'ForkTsCheckerWebpackPlugin'

Issue: Constructor names can be minified in production builds, causing this filter to fail.

Recommendation: Use instanceof checks or plugin-specific properties instead.

Low Priority

7. I18n Translation Files (client/app/libs/i18n/)

The new translation files appear to be stub/placeholder data:

• default.js and translations.js have minimal, hardcoded strings
• No clear integration with the rest of the application

Recommendation:

• Add comments explaining how these integrate with the app
• Consider if these should be in a config/locale directory instead
• Add tests for i18n functionality

8. Missing Error Handling (config/rspack/serverRspackConfig.js:19-23)

if (!serverEntry['server-bundle']) {
  throw new Error(
    "Create a pack with the file name 'server-bundle.js' containing all the server rendering files",
  );
}

Enhancement: This error message could be more helpful by suggesting where to create the file:

throw new Error(
  "Server bundle entry 'server-bundle' not found. Create a pack at client/app/packs/server-bundle.js containing all server rendering files",
);

9. Ruby Version Patch Mismatch (Gemfile.lock)

ruby 3.4.6p32

The patch level changed from p54 to p32, which seems like a downgrade. This might be intentional but should be verified.

🔒 Security Considerations

✅ No security issues identified

• No secrets or credentials in the code
• Dependencies are from trusted sources
• No arbitrary code execution vulnerabilities
• Proper file path handling (though could be improved as noted above)

⚡ Performance Considerations

✅ Performance improvements are excellent

• Rspack provides 2-10x faster cold builds
• 5-20x faster incremental builds
• Proper optimization settings for production
• Server bundle minimization is appropriately disabled

Minor optimization: Consider adding build caching configuration in production.js for even better performance.

🧪 Test Coverage

Concerns:

• No new tests added for the Rspack configuration
• No tests for the i18n translation files
• The PR mentions tests were run successfully, but no test changes are included

Recommendation: Consider adding integration tests that verify:

• Both client and server bundles build successfully
• CSS modules work correctly in both bundles
• HMR functionality works in development
• Production builds are optimized correctly

📝 Code Quality

Overall: Good quality with room for improvement

Positives:

• Clear separation of concerns
• Good use of comments
• Consistent naming conventions
• Proper error messages

Areas for improvement:

• Reduce code duplication
• Add more defensive programming
• Standardize patterns across files
• Add JSDoc comments for exported functions

🎯 Recommendation

APPROVE with minor revisions

This is a solid migration that maintains backward compatibility while providing significant performance benefits. The high-priority issues should be addressed to prevent potential runtime errors, but they don't block the merge given that the builds are passing.

Before Merging:

1. Fix the null reference issue in serverRspackConfig.js:104-106
2. Address the mutable global object concern in commonRspackConfig.js
3. Consider adding the file path validation for robustness

Post-Merge:

1. Add integration tests for the Rspack configuration
2. Refactor duplicate code in serverRspackConfig.js
3. Document the i18n integration
4. Consider removing old webpack configs if no longer needed

Great work on this migration! The performance improvements will be very valuable for the development experience.

🤖 Generated with Claude Code

Pull Request Review: Shakapacker 9.1.0 & Rspack Migration

Great work on this major infrastructure upgrade! This is a well-executed migration that brings significant performance improvements. Below is my detailed review:

✅ Strengths

1. Excellent Migration Strategy

   • Clean separation of Rspack configs mirroring the webpack structure
   • Maintains backward compatibility - webpack configs remain intact
   • Comprehensive testing across all environments (dev/test/prod)

2. Well-Structured Configuration

   • Good use of functional composition in rspackConfig.js for environment-specific modifications
   • Proper handling of SSR vs client bundles with separate configs
   • Smart handling of mutable global configs via merge() in commonRspackConfig.js:69

3. SSR Configuration (serverRspackConfig.js)

   • Correctly preserves CSS module settings while adding exportOnlyLocals: true (lines 95-100)
   • Appropriate devtool choice (eval) for faster SSR builds
   • Proper chunk limitation for server bundles

4. Good Developer Experience

   • Added ReScript .bs.js extension support for proper module resolution
   • Fixed React runtime to 'classic' for SSR compatibility
   • Proper HMR configuration with React Refresh

⚠️ Issues & Recommendations

1. Critical: Potential Runtime Error in serverRspackConfig.js

Location: config/rspack/serverRspackConfig.js:104-106

} else if (rule.use && (rule.use.loader === 'url-loader' || rule.use.loader === 'file-loader')) {
  rule.use.options.emitFile = false;
}

Issue: This will throw an error if rule.use.options is undefined.

Fix:

} else if (rule.use && (rule.use.loader === 'url-loader' || rule.use.loader === 'file-loader')) {
  rule.use.options = rule.use.options || {};
  rule.use.options.emitFile = false;
}

2. Code Quality: Typo in rspackConfig.js

Location: config/rspack/rspackConfig.js:13

// For HMR, need to separate the the client and server rspack configurations

Fix: Remove duplicate "the"

// For HMR, need to separate the client and server rspack configurations

3. Inconsistent Module Mutation Pattern

Location: config/rspack/commonRspackConfig.js:61-63

While you correctly use merge() to avoid mutating globals when creating the config, you're still mutating cssLoader.options.modules directly:

if (cssLoader?.options?.modules?.namedExport) {
  cssLoader.options.modules.exportLocalsConvention = 'camelCaseOnly';
}

Recommendation: For consistency, consider creating a new object:

if (cssLoader?.options?.modules?.namedExport) {
  cssLoader.options.modules = {
    ...cssLoader.options.modules,
    exportLocalsConvention: 'camelCaseOnly'
  };
}

4. Missing Error Handling

Location: config/rspack/commonRspackConfig.js:39-51

If sassLoader is found but has an unexpected structure, the code could fail silently or throw.

Recommendation: Add validation:

if (sassLoaderIndex !== -1) {
  const sassLoader = scssRule.use[sassLoaderIndex];
  if (typeof sassLoader === 'string') {
    scssRule.use[sassLoaderIndex] = {
      loader: sassLoader,
      options: { api: 'modern' }
    };
  } else if (sassLoader && typeof sassLoader === 'object') {
    sassLoader.options = sassLoader.options || {};
    sassLoader.options.api = 'modern';
  } else {
    console.warn('Unexpected sass-loader structure:', sassLoader);
  }
}

5. i18n Translation Files - Missing Usage

Location: client/app/libs/i18n/default.js & translations.js

The new i18n files export defaultMessages and translations, but I see existing code imports from libs/i18n/default. Good! However:

• translations.js exports a translations object but it's not imported/used anywhere
• Consider if both files are needed or if they should be consolidated

Recommendation: Either:

1. Export both from a single file, or
2. Remove translations.js if only defaultMessages is used

6. Security Consideration: New Dependencies

The PR adds significant new dependencies:

• @rspack/core, @rspack/cli - Core bundler (expected)
• rspack-manifest-plugin - For manifest generation
• Various @rspack/binding-* - Platform-specific bindings

Recommendation: Consider:

• Running npm audit to check for vulnerabilities
• Documenting minimum platform requirements (some bindings are platform-specific)
• Verifying license compatibility for production use

7. Performance Testing

Location: PR description mentions "2-10x faster builds"

Recommendation: Consider adding:

• Build time benchmarks to CI/CD (could track regression)
• Memory usage comparison (Rspack should be more efficient)
• Bundle size comparison to ensure no bloat

8. Documentation Gap

Missing:

• Migration guide for other contributors
• Rollback instructions if issues arise
• Updated webpack → rspack in relevant docs

Recommendation: Add a docs/rspack-migration.md with:

• What changed and why
• How to troubleshoot common issues
• How to revert if needed (switching assets_bundler back to webpack)

🔍 Testing Recommendations

1. SSR Testing: Verify server-side rendering works correctly with:

   • CSS modules (both named exports and locals)
   • React components
   • i18n integration

2. HMR Testing: Confirm Hot Module Replacement works in development:

   • JS changes hot reload
   • CSS/SCSS changes hot reload
   • No full page refresh needed

3. Production Build: Verify production bundle:

   • Correct minification
   • Source maps generation
   • No dev-only code leaked

4. ReScript Integration: Test that ReScript compiled .bs.js files resolve correctly from node_modules

📊 Overall Assessment

Code Quality: ⭐⭐⭐⭐☆ (4/5)

• Well-structured, follows good patterns
• Minor issues with error handling

Security: ⭐⭐⭐⭐☆ (4/5)

• New dependencies need audit verification
• No obvious security issues in code

Performance: ⭐⭐⭐⭐⭐ (5/5)

• Excellent performance improvements expected
• Smart configuration choices (eval for SSR, etc.)

Test Coverage: ⭐⭐⭐☆☆ (3/5)

• Manual testing done, but could benefit from automated tests

🎯 Verdict

This PR is very close to merge-ready once the critical issue in serverRspackConfig.js:104-106 is fixed. The other recommendations are nice-to-haves that can be addressed in follow-up PRs.

Required before merge:

• Fix rule.use.options undefined error in serverRspackConfig.js

Recommended before merge:

• Run security audit on new dependencies
• Add basic rollback documentation

Can be follow-up PRs:

• Consolidate i18n files
• Add build performance metrics
• Comprehensive migration documentation

Great work on this migration! The performance benefits will be substantial. 🚀

Review generated with analysis of 18 changed files across ~900 additions

PR Review: Update to Shakapacker 9.1.0 and migrate to Rspack

Summary

This PR successfully migrates from Shakapacker 9.0.0-beta.8 to 9.1.0 stable and transitions from webpack to Rspack as the bundler. The migration is well-structured with comprehensive configuration files and good iterative fixes for edge cases.

✅ Strengths

1. Excellent Migration Approach

• Clean separation of client and server configurations
• Proper handling of mutable global configs using merge()
• Good documentation in code comments explaining rationale

2. Thorough Edge Case Handling

• Fixed ReScript .bs.js resolution (commit fbc5781)
• Patched third-party dependency issues with patch-package
• Addressed SSR compatibility with React runtime switching
• Fixed CSS modules configuration for server bundles

3. Performance Benefits

• Rspack provides 2-10x faster cold builds and 5-20x faster incremental builds
• This is a significant improvement for developer experience

⚠️ Issues & Recommendations

1. CSS Modules Configuration Logic (config/rspack/commonRspackConfig.js:54-63)

Severity: Medium

The logic for finding and configuring the CSS loader could fail silently:

const cssLoader = scssRule.use.find((loader) => {
  const loaderName = typeof loader === 'string' ? loader : loader?.loader;
  return loaderName?.includes('css-loader');
});

if (cssLoader?.options?.modules?.namedExport) {
  cssLoader.options.modules.exportLocalsConvention = 'camelCaseOnly';
}

Issues:

• If cssLoader is a string (not an object), the assignment will fail
• No validation that cssLoader.options is mutable
• The condition only applies the fix when namedExport is true, but the setting might be needed regardless

Recommendation:

if (cssLoader && typeof cssLoader === 'object' && cssLoader.options?.modules?.namedExport) {
  cssLoader.options = cssLoader.options || {};
  cssLoader.options.modules = cssLoader.options.modules || {};
  cssLoader.options.modules.exportLocalsConvention = 'camelCaseOnly';
}

2. Sass Loader Configuration (config/rspack/commonRspackConfig.js:39-52)

Severity: Low

Similar pattern with potential type safety issues when sassLoader might be a string vs object.

Current code:

if (typeof sassLoader === 'string') {
  scssRule.use[sassLoaderIndex] = {
    loader: sassLoader,
    options: {
      api: 'modern'
    }
  };
} else {
  sassLoader.options = sassLoader.options || {};
  sassLoader.options.api = 'modern';
}

This is actually well-handled! No changes needed here.

3. Server Bundle CSS Modules Config (config/rspack/serverRspackConfig.js:95-101)

Severity: Medium

The spread operator preserves existing config, but there's a potential issue:

if (cssLoader && cssLoader.options && cssLoader.options.modules) {
  cssLoader.options.modules = {
    ...cssLoader.options.modules,
    exportOnlyLocals: true,
  };
}

Issue: This assumes cssLoader is an object with an options property. If it's a string, this will fail.

Recommendation:

if (cssLoader && typeof cssLoader === 'object' && cssLoader.options?.modules) {
  cssLoader.options.modules = {
    ...cssLoader.options.modules,
    exportOnlyLocals: true,
  };
}

4. I18n Translation Files (client/app/libs/i18n/)

Severity: Low

The newly added translation files are stub/default implementations:

• default.js and translations.js contain the same English messages
• This creates duplication and potential maintenance issues

Questions:

• Are these files actually being imported/used by the application?
• Should default.js import from translations.js to avoid duplication?

Recommendation:

// default.js
import { translations } from './translations';

export const defaultLocale = 'en';
export const defaultMessages = translations.en;

5. Patch-Package Dependency

Severity: Low - Informational

Using patch-package for @glennsl/rescript-json-combinators is pragmatic but creates maintenance burden:

• The patch needs to be reapplied on every install
• Upstream fix would be better long-term

Recommendation:

• Consider opening an issue/PR with the upstream package
• Document why this patch is needed in a comment or README
• Add the patch file to version control (already done ✓)

6. Ruby Version Mismatch

Severity: Critical - Already Fixed ✓

Great catch fixing the Ruby version mismatch (3.4.3 → 3.4.6) in commit 087ec70. This was blocking CI.

7. Warning Suppression

Severity: Low

ignoreWarnings: [/Module not found: Error: Can't resolve 'react-dom\/client'/]

This suppresses a legitimate warning. Consider:

• Is this a real issue that should be fixed?
• If not, add a comment explaining why it's safe to ignore

🔒 Security Considerations

✅ No security concerns identified:

• Dependency updates are from official packages
• No sensitive data exposure
• Patch-package modifications are transparent and version-controlled
• No dynamic code execution or eval usage beyond standard build tools

🧪 Test Coverage

Observation: No new tests added for the Rspack configuration.

While build configuration typically doesn't have unit tests, consider:

• Integration tests that verify both client and server bundles build successfully
• Tests that SSR works correctly with the new configuration
• The PR description mentions manual testing was done ✓

📋 Documentation

Missing:

• Migration guide for other developers/projects
• Performance benchmarks (actual numbers for this project)
• Rollback procedure if issues arise

Suggestion: Add a docs/rspack-migration.md file documenting:

• Why Rspack was chosen
• Migration steps taken
• Known issues and workarounds
• Performance improvements observed

🎯 Final Verdict

Recommendation: Approve with minor improvements

This is a solid migration PR that demonstrates:

• ✅ Good understanding of the build system
• ✅ Iterative problem-solving approach
• ✅ Comprehensive testing
• ✅ Clear communication in commit messages

The identified issues are mostly edge cases and code quality improvements that can be addressed in a follow-up PR or before merge.

Priority Fixes Before Merge:

1. Add type safety checks for loader configuration (Medium priority)
2. Deduplicate i18n translations (Low priority)
3. Add comment explaining why react-dom/client warning is ignored

Nice to Have (Follow-up PR):

• Documentation for the migration
• Consider upstream PR for rescript-json-combinators
• Performance benchmarks

Great work on this migration! 🚀

Pull Request Review - Shakapacker 9.1.0 + Rspack Migration

Overall Assessment

This is a well-executed migration from Webpack to Rspack with a Shakapacker upgrade. The PR demonstrates thorough testing and maintains backward compatibility. The migration is significant and provides clear performance benefits.

✅ Strengths

1. Excellent Code Organization

• Clean separation between client/server configurations
• Well-structured modular approach mirroring the existing webpack setup
• Configuration files are well-commented with clear explanations

2. Comprehensive Configuration

The Rspack configs properly handle:

• SSR (Server-Side Rendering) with correct exportOnlyLocals for CSS modules
• Development hot reloading setup with React Refresh
• Production optimizations
• Asset handling (images, fonts, etc.)

3. Performance Improvements

• 2-10x faster cold builds
• 5-20x faster incremental builds
• This is a significant developer experience improvement

4. Testing

• All environments tested (development, test, production)
• RuboCop linting passed

⚠️ Issues & Recommendations

1. Critical: Mutable Global Configuration (High Priority)

Location: config/rspack/commonRspackConfig.js:69

const commonRspackConfig = () => merge({}, baseClientRspackConfig, commonOptions, ignoreWarningsConfig);

Issue: The baseClientRspackConfig is generated once at module load time (line 4) and mutated throughout the file (lines 31-65). Even though you're using merge({}, ...), the mutations happen BEFORE the merge, affecting the global object.

Risk: In a multi-process or hot-reload scenario, subsequent builds may have stale or corrupted configuration.

Recommendation:

const commonRspackConfig = () => {
  const baseClientRspackConfig = generateWebpackConfig(); // Generate fresh each time
  const commonOptions = {
    resolve: {
      extensions: ['.css', '.ts', '.tsx', '.bs.js'],
    },
  };
  
  // ... rest of the mutations
  
  return merge({}, baseClientRspackConfig, commonOptions, ignoreWarningsConfig);
};

2. Defensive Programming (Medium Priority)

Location: config/rspack/commonRspackConfig.js:32-52

The code assumes the structure of loader objects but could fail silently if the structure changes.

Recommendation: Add error handling:

if (sassLoaderIndex !== -1) {
  const sassLoader = scssRule.use[sassLoaderIndex];
  if (typeof sassLoader === 'string') {
    scssRule.use[sassLoaderIndex] = {
      loader: sassLoader,
      options: { api: 'modern' }
    };
  } else if (sassLoader && typeof sassLoader === 'object') {
    sassLoader.options = sassLoader.options || {};
    sassLoader.options.api = 'modern';
  }
} else {
  console.warn('sass-loader not found in SCSS rule configuration');
}

3. Security: Ignoring Module Resolution Warnings (Medium Priority)

Location: config/rspack/commonRspackConfig.js:20

ignoreWarnings: [/Module not found: Error: Can't resolve 'react-dom\/client'/],

Issue: Suppressing module resolution errors can mask real dependency issues.

Questions:

• Why can't react-dom/client be resolved?
• Is this a version compatibility issue?
• Should the dependency be added or is this expected for SSR-only builds?

Recommendation: Add a comment explaining why this warning is safe to ignore, or better yet, fix the underlying issue.

4. Code Duplication (Low Priority)

Locations:

• config/rspack/serverRspackConfig.js:30-36 and 72-82
• Similar logic for filtering mini-css-extract-plugin

Recommendation: Extract to a shared utility function:

function removeMiniCssExtract(loaders) {
  return loaders.filter((item) => {
    const testValue = typeof item === 'string' ? item : item?.loader;
    return !(testValue?.match(/mini-css-extract-plugin/) || testValue === 'style-loader');
  });
}

5. Inconsistent Error Handling (Low Priority)

Location: config/rspack/commonRspackConfig.js:27-28

Uses console.warn when SCSS config not found, but:

• config/rspack/serverRspackConfig.js:19-22 throws an error for missing server-bundle

Recommendation: Be consistent - either throw errors for critical configuration issues or handle gracefully with warnings.

6. i18n Files Added Without Context (Medium Priority)

Location:

• client/app/libs/i18n/default.js
• client/app/libs/i18n/translations.js

Issue: These files appear to be new additions but:

• No description in PR of why they're needed
• No usage found in the diff
• Are they related to Rspack migration or a separate feature?
• Missing tests for i18n functionality

Recommendation:

• Clarify if these should be in this PR or a separate one
• Add usage examples or tests
• Consider using established i18n libraries like react-intl or i18next

7. Ruby Version Discrepancy in Gemfile.lock

Location: Gemfile.lock:505

- ruby 3.4.6p54
+ ruby 3.4.6p32

Issue: The patch level changed from p54 to p32 (going backwards?). This seems unusual.

Question: Is this intentional? Could indicate the lock file was generated on a different machine with an older Ruby patch.

🧪 Test Coverage Concerns

Missing Tests for:

1. New Rspack configuration loading and generation
2. Client vs Server bundle separation logic
3. Environment-specific configuration (dev/prod/test)
4. i18n default and translation files
5. The patch-package integration

Recommendation: Consider adding:

• Unit tests for configuration generators
• Integration tests that verify bundles are created correctly
• Tests for i18n utilities if they're being used

🔒 Security Considerations

Positive:

• No credentials or secrets in configs ✅
• Using official Rspack packages ✅
• Dependency versions are specific, not using wildcards ✅

To Verify:

• The patch file patches/@glennsl+rescript-json-combinators+1.4.0.patch modifies a third-party package
  • Verify this patch is necessary and from a trusted source
  • Document why the patch is needed
  • Consider opening an issue/PR with the upstream package

📊 Performance & Best Practices

Excellent:

• Proper SSR configuration with exportOnlyLocals: true for CSS modules
• Correct use of LimitChunkCountPlugin for server bundle
• Using eval devtool for SSR (good for debugging)
• Proper separation of concerns (client/server/common configs)

Consider:

• Monitor bundle sizes before/after to ensure no regression
• Consider adding bundle analysis tooling (webpack-bundle-analyzer equivalent for Rspack)

📝 Documentation Recommendations

1. Add Migration Guide: Document the migration process for other developers
2. Update README: Mention Rspack is now the bundler
3. Rollback Plan: Document how to rollback to webpack if issues arise
4. Configuration Comments: Add comments explaining commented-out lines for React on Rails Pro

✅ Action Items Before Merge

Must Fix:

1. ⚠️ Fix the mutable global configuration issue in commonRspackConfig.js
2. ⚠️ Clarify/remove i18n files or explain their purpose
3. ⚠️ Investigate Ruby version discrepancy in Gemfile.lock
4. ⚠️ Document why react-dom/client warning is ignored

Should Consider:

5. Add defensive null checks in loader configuration
6. Extract duplicated filter logic
7. Add tests for new configuration code
8. Document the rescript-json-combinators patch necessity

Nice to Have:

9. Add bundle size comparison
10. Update documentation about the Rspack migration

🎯 Conclusion

This is a high-quality PR that demonstrates careful planning and execution. The migration is well-structured and maintains feature parity with the webpack configuration.

The main concerns are around defensive programming and the mutable configuration issue which should be addressed before merge. The i18n files and Ruby version change need clarification.

After addressing the critical issues, this PR will be ready to merge and will provide significant performance improvements to the development workflow.

Recommendation: Request changes for the critical issues, then approve after fixes.

Great work on this migration! 🚀

Pull Request Review: Shakapacker 9.1.0 & Rspack Migration

Overall Assessment

This is a well-executed migration that upgrades Shakapacker from beta to stable and successfully migrates from webpack to Rspack. The implementation is thorough, follows best practices, and maintains backward compatibility. The PR demonstrates careful planning and testing.

✅ Strengths

1. Excellent Configuration Architecture

• Well-structured Rspack configuration files with clear separation of concerns:
  • commonRspackConfig.js - Shared configuration
  • clientRspackConfig.js - Client-specific settings
  • serverRspackConfig.js - SSR-specific settings
  • Environment-specific configs (development.js, production.js, test.js)
• Proper use of immutability patterns with merge() to avoid global config mutation

2. Strong SSR Configuration

• Correct handling of CSS modules with exportOnlyLocals: true for SSR (serverRspackConfig.js:95-101)
• Proper devtool setting (eval) for faster SSR builds with good debugging
• Appropriate plugin filtering to avoid conflicts between client and server bundles

3. Backward Compatibility

• CSS loader configuration maintains default exports instead of named exports for compatibility (commonRspackConfig.js:23-37)
• sass-loader configured to use modern API while maintaining existing functionality
• Webpack configs remain in place for reference

4. Good Performance Optimizations

• Rspack provides 2-10x faster cold builds and 5-20x faster incremental builds
• No chunk splitting for server bundle (optimizes SSR)
• React Refresh plugin properly configured for HMR

⚠️ Issues & Recommendations

1. Critical: Duplicate Filter Logic (config/rspack/serverRspackConfig.js)

Lines 27-36 and 72-83 contain duplicate logic for removing mini-css-extract-plugin:

// This block appears twice with slight variations
serverRspackConfig.module.rules.forEach((loader) => {
  if (loader.use && loader.use.filter) {
    loader.use = loader.use.filter(
      (item) => !(typeof item === 'string' && item.match(/mini-css-extract-plugin/)),
    );
  }
});

Recommendation: Remove the first occurrence (lines 27-36) as the second block (lines 72-83) is more comprehensive and handles both mini-css-extract-plugin and style-loader.

2. Security: Dependency Versions

Several new dependencies use caret ranges which could introduce breaking changes:

"@rspack/core": "^1.5.8",
"@rspack/cli": "^1.5.8",
"rspack-manifest-plugin": "^5.1.0"

Recommendation: Consider pinning to exact versions or using tilde (~) for patch-level updates only in package.json to ensure build reproducibility, especially for production deployments.

3. Incomplete Error Handling (config/rspack/commonRspackConfig.js:43-44)

if (scssConfigIndex === -1) {
  console.warn('No SCSS rule with use array found in rspack config');
} else {
  // ... continues with logic that assumes SCSS rule exists

Issue: The code continues execution after warning about missing SCSS config, which will cause runtime errors.

Recommendation: Either throw an error or return early:

if (scssConfigIndex === -1) {
  throw new Error('No SCSS rule with use array found in rspack config. Please check your Shakapacker configuration.');
}

4. New I18n Files Without Usage (client/app/libs/i18n/)

Two new files were added: